Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add Logging SDK implementation for Logging API #386

Merged
merged 1 commit into from
Nov 17, 2020
Merged

Add Logging SDK implementation for Logging API #386

merged 1 commit into from
Nov 17, 2020

Conversation

MarkSeufert
Copy link
Contributor

@MarkSeufert MarkSeufert commented Nov 6, 2020 •
edited
Loading

This PR contains an initial implementation of the Logging SDK, which follows from the initial API commit and the logging prototype issue.

This PR adds:

  • SDK LoggerProvider class - returns instances of the SDK Logger and stores a common processor
  • SDK Logger class - filters logs based off severity and sends to a processor
  • Processor Interface - interface that defines what a processor implementation will require. A future PR will contain concrete processor classes derived from this
  • Unit tests for the LoggerProvider and Logger classes

Any suggestions or feedback would be greatly appreciated! The design doc for the Logging SDK will be released in a future PR.

cc @xukaren @alolita @reyang @ThomsonTan @maxgolov @tigrannajaryan

@MarkSeufert MarkSeufert requested a review from a team November 6, 2020 02:15
@codecov
Copy link

codecov bot commented Nov 6, 2020 •
edited
Loading

Codecov Report

Merging #386 (d33ffdb) into master (fd8527b) will increase coverage by 0.00%.
The diff coverage is 94.25%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #386   +/-   ##
=======================================
  Coverage   95.01%   95.01%           
=======================================
  Files         164      171    +7     
  Lines        7099     7186   +87     
=======================================
+ Hits         6745     6828   +83     
- Misses        354      358    +4
Impacted Files Coverage Δ
sdk/test/logs/logger_provider_sdk_test.cc 90.32% <90.32%> (ø)
sdk/test/logs/logger_sdk_test.cc 91.66% <91.66%> (ø)
sdk/include/opentelemetry/sdk/logs/logger.h 100.00% <100.00%> (ø)
...k/include/opentelemetry/sdk/logs/logger_provider.h 100.00% <100.00%> (ø)
sdk/include/opentelemetry/sdk/logs/processor.h 100.00% <100.00%> (ø)
sdk/src/logs/logger.cc 100.00% <100.00%> (ø)
sdk/src/logs/logger_provider.cc 100.00% <100.00%> (ø)
... and 5 more

lalitb
lalitb reviewed Nov 9, 2020
View reviewed changes
sdk/src/logs/logger.cc Outdated Show resolved Hide resolved
@MarkSeufert MarkSeufert changed the title Initial Commit of Logging SDK Add Logging SDK implementation for Logging API Nov 9, 2020
kxyr
kxyr reviewed Nov 9, 2020
View reviewed changes
sdk/src/logs/logger.cc Outdated Show resolved Hide resolved
pyohannes
pyohannes reviewed Nov 10, 2020
View reviewed changes
Copy link
Contributor

@pyohannes pyohannes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for all your great work on this. I have a few question/suggestions/opinions below.

sdk/include/opentelemetry/sdk/logs/logger.h Outdated Show resolved Hide resolved
sdk/include/opentelemetry/sdk/logs/logger.h Outdated Show resolved Hide resolved
sdk/include/opentelemetry/sdk/logs/logger_provider.h Outdated Show resolved Hide resolved
sdk/src/logs/logger.cc Outdated Show resolved Hide resolved
sdk/src/logs/logger.cc Show resolved Hide resolved
sdk/src/logs/logger.cc Outdated Show resolved Hide resolved
sdk/src/logs/logger_provider.cc Outdated Show resolved Hide resolved
jsuereth
jsuereth approved these changes Nov 11, 2020
View reviewed changes
Copy link
Contributor

@jsuereth jsuereth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a great first PR for logging!

My high level comments:

  • I saw TODOs about pulling LogRecord in via Heap. I'd also love to see support for pulling from a ring/mempool or some other mechanism to limit overall memory consumption from log-processing. We can discuss that in any follow-on PRs / SiG. (This is a nice-to-have)
  • I have some reservation around the shared map + mutex currently. I saw the discussion and I think you're aware of the nuance you're in, I expect we'll want to throw some benchmarks around that code and tune it over time. If we find GetLogger is frequently used in user-code we'll want to make sure that's as efficient as it can be (e.g. components which instantiate on every incoming HTTP Request means we're calling it often, and on many threads). I think you should open a ticket around bench-marking and determining if we should try to migrate to RW lock.
  • Tactically, how are you tracking TODOs and follow on work? Is it feasible to link TODOs to issues on the project? Two reason for this:
    1. It'd be nice to understand what you plan to tackle yourself in follow-on PRs
    2. It'd be nice for others in the community to see whether or not a TODO is actively worked on or if they can pick it up.

sdk/include/opentelemetry/sdk/logs/logger_provider.h Outdated Show resolved Hide resolved
sdk/include/opentelemetry/sdk/logs/logger_provider.h Show resolved Hide resolved
sdk/src/logs/logger.cc Outdated Show resolved Hide resolved
@maxgolov
Copy link
Contributor

maxgolov commented Nov 11, 2020 •
edited
Loading

We discussed the issue of propagating the logger name (or log name) from API to SDK to the protocol layer. @MarkSeufert @xukaren , correct me if I'm wrong on this one, it seems like Log SIG has generally been supportive of idea of NOT dropping the logger name, as the information flows from API->SDK->Exporter. I don't consider this blocking for your PR, but it would be nice-to-have the ability to retain the name on logger object. That way you do not need to do any additional lookup when Log Record gets to exporter. There is no precise place decided, but my goal is - if I plug in a custom protocol exporter (not necessarily OTLP exporter), I should be able to easily fetch the logger or log (not event!) name and populate it somewhere in my own exporter. Log SIG notes. Recording of what's been discussed should be available on YouTube later today.

reyang
reyang requested changes Nov 12, 2020
View reviewed changes
Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. There are several unnecessary methods from the Logger class, need to remove them (we can add them later if there is a need).
  2. The processor ownership seems incorrect, need to move it to the provider.

ThomsonTan
ThomsonTan reviewed Nov 13, 2020
View reviewed changes
sdk/src/logs/logger.cc Outdated Show resolved Hide resolved
reyang
reyang approved these changes Nov 16, 2020
View reviewed changes
Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@reyang reyang merged commit 88d43a3 into open-telemetry:master Nov 17, 2020
This was referenced Dec 1, 2020
Closed
Closed
This was referenced Dec 9, 2020
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxgolov maxgolov maxgolov left review comments

@kxyr kxyr kxyr left review comments

@reyang reyang reyang approved these changes

@pyohannes pyohannes pyohannes left review comments

@lalitb lalitb lalitb left review comments

@ThomsonTan ThomsonTan ThomsonTan left review comments

@jsuereth jsuereth jsuereth approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@MarkSeufert @maxgolov @jsuereth @ThomsonTan @lalitb @pyohannes @reyang @kxyr